Skip to content

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Aug 5, 2025

@yhabteab yhabteab requested a review from julianbrost August 5, 2025 12:10
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Aug 5, 2025
@yhabteab yhabteab force-pushed the sync-image-readme-md branch 3 times, most recently from 6454960 to 907df68 Compare August 5, 2025 13:07
@yhabteab yhabteab changed the title Add a reusable GHA for syncing README.md to Docker Hub Add a step for syncing README.md to Docker Hub in container-image.yml Aug 5, 2025
@yhabteab yhabteab force-pushed the sync-image-readme-md branch from 907df68 to c3615ec Compare August 5, 2025 13:24
# Check if the README file has been modified since the github.event.before reference point
# and write the result to the README_MODIFIED ENV variable.
if git diff --quite --exit-code ${{ github.event.before }} "${{ env.README_FILEPATH }}"; then

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if git diff --quite --exit-code ${{ github.event.before }} "${{ env.README_FILEPATH }}"; then
if git diff --quiet --exit-code ${{ github.event.before }} "${{ env.README_FILEPATH }}"; then

Also, aren't the cases the wrong way around? Exit code 0 (i.e. true) means no difference.

@julianbrost
Copy link

Other idea (originating from Icinga/icinga2#10505 (comment)): The documentation mostly describes the latest tag, like the examples refer to something like icinga/icinga2 (implicitly :latest), so what about syncing when uploading that tag (i.e. on: release) instead of for pushes to the default branch?

@yhabteab yhabteab force-pushed the sync-image-readme-md branch from c3615ec to cadb12d Compare August 8, 2025 11:52
@yhabteab yhabteab requested a review from julianbrost August 8, 2025 11:53
Comment on lines 213 to 221
# Check if the README file has been modified since the github.event.before reference point
# and write the result to the README_MODIFIED ENV variable.
if ! git diff --quiet --exit-code ${{ github.event.before }} "${{ env.README_FILEPATH }}"; then
echo "README file has been modified since the last commit."
echo "README_MODIFIED=true" >> "$GITHUB_ENV"
else
echo "README file has not been modified since the last commit."
echo "README_MODIFIED=false" >> "$GITHUB_ENV"
fi

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does ${{ github.event.before }} even exist for release events? Either way, I'd just cut down on complexity here and push the sync it with every release. We aren't releasing that frequently after all that I'd expect any trouble there. Or does Docker Hub actually complain if you upload the very same file again?

container_readme_filepath:
required: false
type: string
description: 'Path to the README file to sync with Docker Hub. Defaults to the first For-Container.md file found in the ./doc/ directory.'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first For-Container.md

The implementation uses find and that guarantees no ordering, so if there were multiple files, one would basically be picked at random.

Comment on lines 196 to 200
file_path=$(find ./doc/ -type f -name 'For-Container.md' | head -n 1)
if [ -z "$file_path" ]; then
echo "No For-Container.md file found in the ./doc/ directory."
exit 1
fi
Copy link

@julianbrost julianbrost Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change the implementation so that either README_FILEPATH is given, or there must be exactly one For-Container.md inside doc/.

Hint: if you do the following, you'll have all matches in an array and can easily check its length:

shopt -s globstar # enable ** for recursive glob search
files=(doc/**/For-Container.md)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ shopt -s globstar
$ files=a=$(doc/**/For-Container.md)
bash: doc/02-installation.md.d/For-Container.md: Permission denied

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, of course! Ignore my previous comment :)! This is the actual error I get when I copy paste your commands:

$ shopt -s globstar
$ files=a=(doc/**/For-Container.md)
bash: syntax error near unexpected token `('

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm too stupid to copy & paste 😅 I tried this locally with variable name a then thought "would be nice to use a proper variable name" and prepended it instead of replacing it. There should be no a= in there, see my edited comment above now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'v already changed the PR, so no need to mess around with bash magics!

@yhabteab yhabteab force-pushed the sync-image-readme-md branch from cadb12d to 6f9b5e4 Compare August 29, 2025 16:21
@julianbrost julianbrost self-assigned this Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants